Decoupling file excludability from mutant spans #2645
Conversation
src/Stryker.Core/Stryker.Core.UnitTest/DiffProviders/GitDiffProviderTests.cs
Show resolved
Hide resolved
src/Stryker.Core/Stryker.Core.UnitTest/MutantFilters/FilePatternMutantFilterTests.cs
Show resolved
Hide resolved
src/Stryker.Core/Stryker.Core/ProjectComponents/ExcludableProjectComponent.cs
Show resolved
Hide resolved
| return pattern.MutantSpans.Any(span => RangeModule.rangeContainsRange(FromMutantSpan(span), range)); | ||
| } | ||
|
|
||
| public override IEnumerable<Range> Reduce(IEnumerable<Range> spans) |
There was a problem hiding this comment.
Actually much will be probably taken from the TextSpanHelper, eventually using the same model as is done for span matching.
There was a problem hiding this comment.
Will the TextSpanHelper be the abstraction for span and range?
There was a problem hiding this comment.
Well as of now TextSpanHelper is a set of extensions on top of TextSpan (C#) whereas RangeHelper is a set of extensions on top of Range (F#). But let me see if I can refactor it a bit, there is some common code to extract.
|
|
||
| namespace Stryker.Core.ProjectComponents | ||
| { | ||
| public class SimpleFileLeaf: ProjectComponent, IReadOnlyFileLeaf |
There was a problem hiding this comment.
This is really needed only GitDiffProvider. Although it's useful in tests also.
Now, I guess this could be somehow inserted in the tree of abstractions here since there is some logic duplication with ExcludableProjectComponent. But my brain already hurts from abstractions here and the duplication is rather small anyway.
There was a problem hiding this comment.
I'm not sure what this class is doing.. Is this a non-language-specific file? Should this be an abstract class?
There was a problem hiding this comment.
Correct, this is a non-language specific file. It's initiated in the GitDiffProvider where we indeed don't care about the code:
private void RemoveFilteredOutFiles(DiffResult diffResult)
{
foreach (var glob in new SimpleFileLeaf(_options.DiffIgnoreChanges).Patterns.Select(d => d.Glob))
{
diffResult.ChangedSourceFiles = diffResult.ChangedSourceFiles.Where(diffResultFile => !glob.IsMatch(diffResultFile)).ToList();
diffResult.ChangedTestFiles = diffResult.ChangedTestFiles.Where(diffResultFile => !glob.IsMatch(diffResultFile)).ToList();
}
}| @@ -78,7 +78,13 @@ private void DisplayComponent(IReadOnlyProjectComponent inputComponent, Table ta | |||
|
|
|||
| var mutationScore = inputComponent.GetMutationScore(); | |||
|
|
|||
| if (inputComponent.IsComponentExcluded(_options.Mutate)) | |||
| var isExcluded = inputComponent switch | |||
There was a problem hiding this comment.
Trying to understand this... is this correct? Can a folder be excluded in the context of clear text reporters?
There was a problem hiding this comment.
Yes, with globbing it's possible to exclude a whole folder. So it will be displayed as excluded. However, now that you mention this, we moved from exclude to ignore as the preferred term. So maybe we need to display [Ignored] instead of [Excluded].
|
Unit tests pass, there are two integration tests failing, I will look into them. Maintainers - please, before I continue on this. Do you think this is a reasonable design change? Is the current change something you approve "in principle"? :) |
|
Ping @richardwerkman @rouke-broersma @dupdob :) |
There was a problem hiding this comment.
Do you think this is a reasonable design change? Is the current change something you approve "in principle"? :)
I think this is generally going in the right direction. I have some questions regarding the abstractions, but those are kind of a mess anyway.
We could also choose to not implement some of the deeper features in F#, like ignoring part of a file. If this makes the transition to F# easier. We can always implement it later
| @@ -78,7 +78,13 @@ private void DisplayComponent(IReadOnlyProjectComponent inputComponent, Table ta | |||
|
|
|||
| var mutationScore = inputComponent.GetMutationScore(); | |||
|
|
|||
| if (inputComponent.IsComponentExcluded(_options.Mutate)) | |||
| var isExcluded = inputComponent switch | |||
There was a problem hiding this comment.
Yes, with globbing it's possible to exclude a whole folder. So it will be displayed as excluded. However, now that you mention this, we moved from exclude to ignore as the preferred term. So maybe we need to display [Ignored] instead of [Excluded].
| return pattern.MutantSpans.Any(span => RangeModule.rangeContainsRange(FromMutantSpan(span), range)); | ||
| } | ||
|
|
||
| public override IEnumerable<Range> Reduce(IEnumerable<Range> spans) |
There was a problem hiding this comment.
Will the TextSpanHelper be the abstraction for span and range?
|
|
||
| namespace Stryker.Core.ProjectComponents | ||
| { | ||
| public class SimpleFileLeaf: ProjectComponent, IReadOnlyFileLeaf |
There was a problem hiding this comment.
I'm not sure what this class is doing.. Is this a non-language-specific file? Should this be an abstract class?
| @@ -0,0 +1,17 @@ | |||
| namespace Stryker.Core.ProjectComponents | |||
| { | |||
| public class ExcludableString | |||
There was a problem hiding this comment.
What does this class do? And is this cSharp specific?
There was a problem hiding this comment.
Well, this is now basically just a non-language-specific construct to represent the idea that we can include or exclude files in some code flows. Distinguish Person.cs vs !Person.cs or Person.fs vs Person.fs. E.g. DiffIgnoreChangesInput don't need info about spans as opposed to MutateInput.
|
Okay sorry for the delay, I am back on this. Now tests pass, int tests pass, good stuff. Let me see if I can refactor things a bit and add some extra tests, I will let you know then :) |
|
Okay, so this one is getting quite big and I promised small PRs :) I extracted the changes to the following PR:
Let's address those ones first and then see what's left here. Meanwhile converting to draft again. |
This is more relevant than ever, due to work being done on a mutation server protocol for driving mutation testing from external sources such as IDE plugins. The protocol supports mutant spans, and defines them as 1-based. It would be practical to take mutant spans as 1-based in configuration and then convert them to whatever csharp and/or f# needs. |
|
Sorry for being a very bad contributor recently 😢 my life was quite turbulent until very recently. Things are settling up tho, also I got a lot of F# knowledge meanwhile and hope to get back on track with this during gray autumn and dark winter evenings. |
No worries, we haven't kicked you out yet ;D Life happens, hope all's well for you soon! |
Phew... Okay so this is a somewhat bigger steps towards the F# support.
Motivation
F# has its own understanding of text documents (btw that's sad). Roslyn (in this context C#) generally counts all characters from zero whereas F# generally uses lines and columns. Of course one is convertible to the other, it's more about what's native to the compiler. Type-wise, it's
Microsoft.CodeAnalysis.Text.TextSpanversusFSharp.Compiler.Text.Range.Sooner or later we would need native span arithmetics for mutants hence one approach is to create a definition of mutant span on the user level (= configuration level) and then translate them to compiler definitions of spans based on the file type.
Implementation
This PR
FilePatternlanguage-agnosticTO DO